Skip to content

add gomobile test2#2

Open
zerojg wants to merge 4 commits intomasterfrom
gomobile-2
Open

add gomobile test2#2
zerojg wants to merge 4 commits intomasterfrom
gomobile-2

Conversation

@zerojg
Copy link
Copy Markdown
Owner

@zerojg zerojg commented Jan 13, 2026

test 2

Repository owner deleted a comment from claude Bot Jan 13, 2026
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libwallet Security & Concurrency Review

Summary:

  • 1 CRITICAL issue (potential deadlock)
  • 1 WARNING (lock held during I/O)
  • 1 SUGGESTION (initialization order)

Security Notes:

✅ No secrets logged
✅ Error messages appropriately generic
✅ Mutex patterns mostly correct

Details:

See inline comments for specific issues.

Comment thread gomobile/gomobile.go
}
wallets = make(map[string]*wallet)

// Stop all remaining background processes and wait for them to stop.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[CRITICAL] Potential deadlock: Context cancellation with mutex held.

Current:

walletsMtx.Lock()
defer walletsMtx.Unlock()
// ...
cancelMainCtx()
wg.Wait()

Issue: If any goroutine in wg tries to acquire walletsMtx (e.g., in wallet operations), it will deadlock because wg.Wait() is called while holding the mutex.

Suggested:

walletsMtx.Lock()
// ... close wallets ...
walletsMtx.Unlock()

// Cancel and wait AFTER releasing the mutex
cancelMainCtx()
wg.Wait()

logMtx.Lock()
// ... close logger ...

Why: Background goroutines must be able to acquire locks during shutdown. Holding locks during wg.Wait() is a common deadlock pattern.

Comment thread gomobile/gomobile.go
log = logBackend.SubLogger("APP")
logMtx.Unlock()

mainCtx, cancelMainCtx = context.WithCancel(context.Background())
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SUGGESTION] Initialization order could be improved.

Current: Context created after setting initialized = true flag (line 77).

Suggested: Move mainCtx, cancelMainCtx = context.WithCancel(context.Background()) to before setting initialized = true (around line 75).

Why: If initialization is checked between line 77 and 76, the context might not be ready. Minor race condition.

Comment thread gomobile/gomobile.go
if !initialized {
return "", errors.New("not initialized")
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[WARNING] RLock held during potentially slow operation.

Current: logMtx.RLock() held during log.Debug() which may involve I/O.

Consider: If logging is fast and non-blocking in this implementation, this may be acceptable. However, best practice is to minimize lock duration.

Suggested pattern (if needed):

logMtx.RLock()
logger := log
logMtx.RUnlock()
logger.Debug("libwallet mobile shutting down")

Why: Avoid holding locks during I/O operations to prevent blocking other goroutines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants